Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fslib): handle .zip in the middle of file name again #3042

Merged
merged 6 commits into from
Jul 18, 2021

Conversation

ratson
Copy link
Contributor

@ratson ratson commented Jun 22, 2021

What's the problem this PR addresses?

Path with .zip in the middle of the file name is not extracted correctly.

Closes #2975

How did you fix it?

Added 2 edge cases to the test.

...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz
Copy link
Member

merceyz commented Jun 22, 2021

For context that function was added in #2796 to speed up installs / replace this regex

.*?(?<!\/)\.zip(?=\/|$)

We need to verify that this fix doesn't regress the performance to the point where we could just use the regex

@ratson
Copy link
Contributor Author

ratson commented Jun 22, 2021

@merceyz
code block 1 is new code,
code block 2 is regex version,

Before:
https://jsben.ch/CTkgN
Screenshot 2021-06-22 at 22-55-08 JSBEN CH Benchmarking for JavaScript -

After:
https://jsben.ch/b07Zt
Screenshot 2021-06-22 at 22-54-03 JSBEN CH Benchmarking for JavaScript -

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be updated to handle an "unlimited" amount of .zip occurrences in a filename

@ratson
Copy link
Contributor Author

ratson commented Jun 23, 2021

@merceyz Which test case is in your mind? The change should have handled multiple .zip occurrences.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an edge case but here is a failing test case

@merceyz merceyz changed the title fix(ZipOpenFS): handle .zip in the middle of file name fix(fslib): handle .zip in the middle of file name again Jun 23, 2021
@ratson ratson force-pushed the fix-zip-path-extraction branch from b730180 to 485f09f Compare June 24, 2021 13:38
@ratson
Copy link
Contributor Author

ratson commented Jun 24, 2021

https://jsben.ch/oBqKF
Screenshot 2021-06-24 at 21-41-00 JSBEN CH Benchmarking for JavaScript -

@ratson ratson force-pushed the fix-zip-path-extraction branch from 485f09f to 64cd587 Compare June 28, 2021 03:49
@ratson ratson dismissed a stale review via ee5f023 July 8, 2021 05:50
@ratson ratson force-pushed the fix-zip-path-extraction branch from 64cd587 to ee5f023 Compare July 8, 2021 05:50
@ratson ratson requested a review from merceyz July 8, 2021 07:05
@arcanis arcanis merged commit 7b97a5d into yarnpkg:master Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] lodash.zip gets confused for a zip archive
3 participants